-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-36043: FileCookieJar supports os.PathLike #11945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Docs should be updated https://docs.python.org/3.8/library/http.cookiejar.html#http.cookiejar.LWPCookieJar with versionchanged markup.
-
NEWS entry can use term:
path-like object
instead of pathlib.Path and FileCookieJar can be linked with markup.
Thanks for the PR.
@tirkarthi done, could you review again? thanks |
Doc/library/http.cookiejar.rst
Outdated
@@ -341,6 +345,9 @@ writing. | |||
compatible with the libwww-perl library's ``Set-Cookie3`` file format. This is | |||
convenient if you want to store cookies in a human-readable file. | |||
|
|||
.. versionchanged:: 3.8 | |||
|
|||
The filename parameter supports a :class:`~pathlib.Path` instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted below this can be anything that implements fspath and not only pathlib.Path instance. Please use path-like object term with markup (:term:path-like object
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Doc/library/http.cookiejar.rst
Outdated
@@ -71,6 +71,10 @@ The following classes are provided: | |||
:meth:`load` or :meth:`revert` method is called. Subclasses of this class are | |||
documented in section :ref:`file-cookie-jar-classes`. | |||
|
|||
.. versionchanged:: 3.8 | |||
|
|||
The filename parameter supports a :class:`~pathlib.Path` instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use path-like object as below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks 👍
Out of curiosity, why inverting the |
@JulienPalard good catch, thank you, just updated with your recommendation. |
Because I stopped to raise an |
Lib/test/test_http_cookiejar.py
Outdated
filename = test.support.TESTFN | ||
c = LWPCookieJar(filename) | ||
self.assertEqual(c.filename, filename) | ||
self.assertIsInstance(c.filename, str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the type checks necessary? Unless the docs explicitly state that then I don't think they are necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, they are not necessary, I have removed them. Thank you for your review.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
Co-Authored-By: matrixise <[email protected]>
@brettcannon if you want to continue your review, thank you. |
@matrixise you forgot to ask Bedevere to mark this as ready to be reviewed again. 😄 Otherwise it doesn't end up in my review queue. |
I have made the requested changes; please review again |
Thanks for making the requested changes! @brettcannon: please review the changes made to this pull request. |
useful this very small sentence for @bedevere-bot ;-) |
@matrixise thanks! |
https://bugs.python.org/issue36043